Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed editor throwing an exception when destroyed shortly after it was created #2258

Closed
wants to merge 12 commits into from
Closed

Fixed editor throwing an exception when destroyed shortly after it was created #2258

wants to merge 12 commits into from

Conversation

mlewand
Copy link
Contributor

@mlewand mlewand commented Jul 25, 2018 •
edited
Loading

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

  • Unit tests
  • Manual tests

Skipped the manual test as this is pretty much an internal change, and the case presented for manually reproduced the issue required refreshing the page.

What changes did you make?

This PR adds two early returns, making sure that delayed editor initialization doesn't take place in case it was removed before.

4 people in total worked on this one :D I had to adjust the unit tests, as they were not failing for previous CKEditor releases, thus it would not protect us from a regression.

This is a follow-up of a #273 PR.

Closes #2257, #718.

@mlewand mlewand added the review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer. label Jul 25, 2018
@mlewand mlewand changed the base branch from major to master July 25, 2018 11:15
msamsel
msamsel suggested changes Jul 25, 2018
View reviewed changes
Copy link
Contributor

@msamsel msamsel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last commit (fix for IE8) makes test always pass.
screen shot 2018-07-25 at 16 00 37

jacekbogdanski
jacekbogdanski requested changes Jul 25, 2018
View reviewed changes
Copy link
Member

@jacekbogdanski jacekbogdanski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we shouldn't add a manual test for this changes. As I understand this issue somehow touched users using CKEditor so IMO we should cover it with a manual test, especially that current unit test is false positive and may give invalid result in the future.

tests/plugins/wysiwygarea/destroy.js Outdated

bender.test( {
// (#2257)
'test destroy editor on instance created': function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test always passes despite changes.

@mlewand
Copy link
Contributor Author

mlewand commented Jul 26, 2018

Fixed the unit test.

I decided to remove wysiwygarea change, as it does not fix the issue, there's still an option to break it with the following code: https://gist.github.com/mlewand/ee09f6dc0148792c8d08f53cc55da605 However fixing it would take more time (it would need to be done very carefully in core setMode logic) and it's just an edge case.

As for manual test I related to that in:

Skipped the manual test as this is pretty much an internal change, and the case presented for manually reproduced the issue required refreshing the page.

jacekbogdanski
jacekbogdanski requested changes Jul 26, 2018
View reviewed changes
Copy link
Member

@jacekbogdanski jacekbogdanski left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unit test is still very unstable on IE8. It fails half of a time.

ie8unstable

@f1ames
Copy link
Contributor

f1ames commented Sep 2, 2019

Closing this PR as there is already more general PR on the way #3128.

@f1ames f1ames closed this Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacekbogdanski jacekbogdanski jacekbogdanski requested changes

@msamsel msamsel Awaiting requested review from msamsel

Assignees
No one assigned
Labels
review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mlewand @f1ames @msamsel @jacekbogdanski @brian-mcnamara @Tade0